Skip to content

Fix unknown type cast crash #994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 1, 2020

Conversation

augusto2112
Copy link

@augusto2112 augusto2112 commented Mar 28, 2020

This PR fixes the crash reported here. In addition, it improves the error message when type reconstruction fails in the SwiftASTContext class.

@augusto2112 augusto2112 changed the title Fix unkwnown type cast crash Fix unknown type cast crash Mar 28, 2020
@augusto2112 augusto2112 force-pushed the fix_crash_unkwnown_type branch from 4038311 to 4b3d055 Compare March 28, 2020 00:33
@augusto2112
Copy link
Author

Hi @vedantk, could you take a look at this, if/when you have the time? Related to this post you helped me with

@vedantk
Copy link

vedantk commented Mar 31, 2020

Hey @augusto2112, thanks for doing this, this seems like the right approach. I noticed that there are quite a few formatting diffs in the file unrelated to your change: perhaps these got pulled in by clang-format? Please update the diff to remove those unrelated changes if possible. It should be good to land afterwards.

@augusto2112
Copy link
Author

Hi @vedantk, I was thinking in doing a small change:

expose a public SwiftASTContext::ReconstructType(ConstString) (no error), and do the whole "if error, add diagnostic" here. That way we don't need AddErrorStatusAsGenericDiagnostic in SwiftASTContext's public API and we remove the "if error" logic that is duplicated in three places.

What do you think?

I noticed that there are quite a few formatting diffs in the file unrelated to your change: perhaps these got pulled in by clang-format?

Yeah, I think it was because of clang-format. I'll update that.

@vedantk
Copy link

vedantk commented Apr 1, 2020

It may be convenient in the future to have AddErrorStatusAsGenericDiagnostic as part of SwiftASTContext's public interface. I think it'd be fine to add the helper, though.

@augusto2112 augusto2112 force-pushed the fix_crash_unkwnown_type branch from 4b3d055 to 2b2672b Compare April 1, 2020 00:41
@augusto2112 augusto2112 force-pushed the fix_crash_unkwnown_type branch from 2b2672b to 0718eea Compare April 1, 2020 00:47
@augusto2112
Copy link
Author

@vedantk OK, I pushed the changes.

@vedantk
Copy link

vedantk commented Apr 1, 2020

@swift-ci test

@vedantk vedantk merged commit e7ae372 into swiftlang:swift/master Apr 1, 2020
@vedantk
Copy link

vedantk commented Apr 1, 2020

@augusto2112 could you open a PR submitting this change to apple:swift/master-next as well? Otherwise, the change will disappear after the next rebase (we do not auto merge lldb changes from swift/master to swift/master-next). If you don't have the bandwidth for that, just lmk, and I'll handle it. Thanks.

@vedantk
Copy link

vedantk commented Apr 1, 2020

(Incidentally if you've done other lldb work on swift/master, this will need to be cherry-picked to swift/master-next as well.)

@augusto2112
Copy link
Author

@vedantk Ok, will do!

(Incidentally if you've done other lldb work on swift/master, this will need to be cherry-picked to swift/master-next as well.)

I've done a couple, I usually wait to see if the tests pass before cherry-picking so I don't have to re-do the master-next in case I have to change something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants